sequenceDiagram
    autonumber
    actor Client
    participant Express as Shared Express App
    participant Router as Version Router
    participant V1 as V1 App Instance
    participant V2 as V2 App Instance
    participant JWKS as JWKS Client (lazy)
    participant Tools as Per-version Tools

    Note over Client,Express: Multi-version request dispatch

    Client->>+Express: POST /v1/mcp
    Express->>+Router: route /v1
    Router->>+V1: forward request
    alt OAuth required
        V1->>+JWKS: resolve/init JWKS (lazy, per-version)
        JWKS-->>-V1: jwks client
        V1->>V1: verify JWT via middleware
    end
    V1->>+Tools: invoke v1 tool
    Tools-->>-V1: return result
    V1-->>-Router: respond
    Router-->>-Express: response
    Express-->>-Client: 200 OK

    Client->>+Express: POST /v2/mcp
    Express->>+Router: route /v2
    Router->>+V2: forward request
    alt OAuth required
        V2->>+JWKS: resolve/init JWKS (lazy, per-version)
        JWKS-->>-V2: jwks client
        V2->>V2: verify JWT via middleware
    end
    V2->>+Tools: invoke v2 tool
    Tools-->>-V2: return result
    V2-->>-Router: respond
    Router-->>-Express: response
    Express-->>-Client: 200 OK

Pre-merge checks and finishing touches

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review: API Versioning Feature

This PR introduces a comprehensive API versioning system. Overall, this is a well-implemented feature with excellent test coverage and documentation.

✅ Strengths

Architecture & Design

• Clean separation between single-version and multi-version code paths
• Efficient shared Express app across versions
• Smart OAuth client reuse using config hashing (createApp.ts:719-723)

Test Coverage

• Comprehensive 710-line test suite covering multi-version creation, config merging, route isolation, and backward compatibility
• Integration tests with real HTTP server and MCP SDK client

Documentation

• Excellent README updates with clear examples
• Updated minimal example demonstrating versioning

🔍 Critical Issues

1. OAuth Config Hashing Not Deterministic (createApp.ts:720)
JSON.stringify(normalizedVersionConfig.config.oauth) is not deterministic for object key ordering. Two identical configs with different key orders will create separate JWKS clients.

Recommendation: Use sorted keys before stringifying or use a proper hash function.

2. Missing JWKS Client Cleanup (createApp.ts:665-918)
Multi-version apps store JWKS clients in a Map but never clean them up. This could prevent process exit and cause memory leaks.

Recommendation: Add cleanup in stop() method or new shutdown() method.

⚠️ High Priority Issues

3. Dead Code in Version Route Validation (createApp.ts:288-293)
The check if (versionRoute === '/health') can never trigger since version keys must match /^v\d+$/.

4. Missing Error Handling in once() (createApp.ts:1022-1063)
The multi-version once() implementation needs try-catch around handler invocation to ensure cleanup happens even on error.

5. Debug Logger Limitation (createApp.ts:704-707)
Version-specific debug configs are ignored (global singleton). Document this clearly or support per-version loggers.

📝 Medium Priority Issues

6. Type Assertion Bypass (createApp.ts:960-970)
return firstVersion as unknown as McpServer bypasses TypeScript safety. Consider refactoring types.

7. Unused Parameter (createApp.ts:979, server/index.ts:339)
The env parameter in handleRequest() is never used. Either implement or remove.

8. Magic Numbers (createApp.ts:764-768)
OAuth config values (600000, 10, 5000) should be extracted as constants.

9. Confusing Naming (types/config.ts:248-278)
VersionConfig has optional config property. Consider renaming to overrides for clarity.

🔒 Security Analysis

✅ No critical security issues found

• Proper OAuth JWKS discovery and token validation
• Input validation via Zod schemas
• Proper route isolation between versions
• Configurable CORS

Minor note: OAuth error messages at createApp.ts:784 might leak sensitive server URLs in production.

📊 Performance Notes

Good characteristics:

• JWKS client reuse optimization
• Lazy OAuth initialization
• Content hashing for UI cache busting

Concerns:

• Synchronous fs.readFileSync() at server/index.ts:978 blocks event loop
• JSON.stringify in OAuth config hashing hot path

🎯 Recommendations

Must Fix:

1. Make OAuth config hashing deterministic
2. Add JWKS client cleanup mechanism

Should Fix:
3. Remove dead validation code or expand it
4. Add error handling in once()
5. Document debug logger limitation

✅ Conclusion

This is a high-quality PR with excellent test coverage (710 lines) and documentation. The architecture is sound and maintains backward compatibility. Address the two critical issues before merging.

Recommendation: Approve with requested changes

Great work! 🎉

@cursor review

Skipping Bugbot: Bugbot is disabled for this repository

@cursor review

Code Review: API Versioning Feature

I've completed a thorough review of PR #63 which adds API versioning support to mcp-apps-kit. This is a well-architected feature with solid implementation.

✅ Strengths

1. Excellent Architecture & Design

• Clean separation between single-version and multi-version apps with backward compatibility
• Smart config merging strategy (version-specific overrides global)
• Proper route isolation with shared Express app
• OAuth JWKS client reuse for identical configs (performance optimization at lines 712-734 in createApp.ts)

2. Robust Validation

• Comprehensive runtime validation of version keys
• Config validation for both global and version-specific settings
• Good error messages with context

3. Strong Test Coverage

• 691 lines of comprehensive unit tests (versioning.test.ts)
• Tests cover: config merging, route isolation, tool execution, middleware, events, backward compatibility
• Integration tests using actual MCP clients and HTTP requests

4. Documentation Quality

• Extensive README updates with clear examples
• Well-documented API in inline comments
• Good examples in minimal/ showing v1 vs v2 differences

🔍 Issues & Concerns

CRITICAL: Event Handler Memory Leak (High Priority)

Location: createApp.ts:1015-1056

The once() implementation for multi-version apps has a race condition that can cause memory leaks. If version A fires the event before version B registers, version B's wrapper stays attached but will never execute (due to fired flag). The handler stays in memory indefinitely.

Fix: Unsubscribe from all versions BEFORE calling the handler to prevent race conditions.

MAJOR: Missing Version Validation (Medium Priority)

Location: createApp.ts:972-992

The handleRequest() method for serverless doesn't validate that the version key exists before routing. Valid version pattern (v999) but non-existent version returns generic 404. Should return explicit Version not found error with available versions.

MINOR Issues:

1. Inconsistent Error Handling (createApp.ts:469-515, 780-783) - OAuth initialization should also reset jwksClient on failure
2. Type Safety Gap (createApp.ts:921) - Double type assertion defeats TypeScript's safety
3. Missing Test Cleanup (versioning.test.ts) - Several tests don't clean up HTTP servers in error paths

📊 Testing Assessment

Coverage: Excellent (691 new test lines)

Missing Tests:

• OAuth config reuse
• Error paths in OAuth initialization
• handleRequest() with invalid version keys
• Event system edge cases (especially once() race conditions)

🔒 Security Review

✅ No security vulnerabilities detected

• OAuth validation properly isolated per version
• CORS config correctly merged and applied
• No injection risks in route parsing
• Input validation maintained for all versions

📝 Recommendation Summary

✅ Approval Status

Conditional Approval - Fix the CRITICAL issue before merge.

This is a well-designed feature that adds significant value to the framework. The implementation is solid, documentation is excellent, and test coverage is strong. However, the event handler race condition in once() must be addressed before merging to prevent memory leaks in production.

After fixing the critical issue, this will be a great addition to mcp-apps-kit! 🎉

Great work overall! The versioning architecture is clean and maintainable. 🚀

Code Review: API Versioning Feature

I've completed a thorough review of this PR. Overall, this is a well-designed and thoughtfully implemented feature that adds significant value to the framework. The code quality is high, tests are comprehensive, and documentation is excellent.

✅ Strengths

Architecture & Design

• Clean abstraction: The distinction between single-version and multi-version apps is well-handled with proper backward compatibility
• Excellent separation of concerns: Version isolation is maintained while sharing common infrastructure
• Smart config merging: Version-specific configs properly override global configs with clear precedence rules
• Lazy initialization: OAuth JWKS clients are initialized on-demand, supporting serverless deployments

Code Quality

• Type safety: Strong TypeScript usage throughout with proper type inference
• Validation: Comprehensive runtime validation for version keys, config structure, and route conflicts
• Error handling: Proper error boundaries and informative error messages
• Clean code patterns: Good use of helper functions, consistent naming, clear comments

Testing

• Comprehensive coverage: 806 lines of tests covering version creation, config merging, route isolation, middleware, and backward compatibility
• Integration tests: Real HTTP server tests verify end-to-end functionality
• Edge cases: Tests cover 404 handling, invalid version keys, and tool execution isolation

Documentation

• Excellent README updates: Clear examples for both single and multi-version apps
• Updated examples: The minimal example demonstrates versioning patterns effectively
• API documentation: Type definitions include helpful JSDoc comments

🔍 Issues & Concerns

1. Memory Management - HTTP Server Leak Risk ⚠️ HIGH PRIORITY

Location: createApp.ts:938-950

The shared HTTP server is created but there's no cleanup mechanism. No app.stop() method for multi-version apps. Risk of port conflicts and resource leaks in test suites.

Recommendation: Add a stop() method to the multi-version app that properly closes the shared HTTP server.

2. Config Merging Shallow vs Deep 🤔 MEDIUM PRIORITY

Location: createApp.ts:329-344

The comment states nested objects are replaced entirely, not deep-merged. This could be surprising behavior where version-specific OAuth configs completely replace global OAuth configs, losing shared properties like scopes.

Recommendation: Document this behavior more prominently in the README with a clear example, or consider deep merging for nested objects like OAuth/CORS configs.

3. Type Safety Gap in Multi-Version Tools ⚠️ MEDIUM PRIORITY

Location: createApp.ts:919-921

The main app tools property uses the first version tools with a type assertion. This is problematic because different versions may have different tool types.

Recommendation: Make tools optional/undefined for multi-version apps to force users to use getVersion(), or add a deprecation notice.

4. OAuth JWKS Client Reuse Logic ⚠️ MEDIUM PRIORITY

Location: createApp.ts:713-787

The OAuth client reuse is based on JSON stringifying the config. JSON.stringify is order-dependent - identical configs with different key orders will create separate clients. The tokenVerifier function won't serialize properly.

Recommendation: Use a more robust hashing approach with sorted keys.

5. Test Cleanup and Port Management ⚠️ MEDIUM PRIORITY

Location: versioning.test.ts (multiple locations)

Tests use hardcoded ports which can cause port conflicts if tests run in parallel. Cleanup logic is repetitive and incomplete.

Recommendation: Use dynamic port allocation, add proper afterEach hooks, and create a test helper function for server lifecycle management.

📊 Overall Assessment

Rating: ⭐⭐⭐⭐½ (4.5/5)

This is a high-quality feature implementation that follows the framework conventions and maintains backward compatibility. The main concerns are around resource management and some edge cases in config merging and OAuth client reuse.

Recommendation: Approve with minor changes

Must fix before merge:

• Add stop() method for multi-version apps
• Improve test cleanup with proper afterEach hooks

Should fix (can be follow-up PRs):

• Document config merging behavior
• Improve OAuth client reuse hashing
• Add more edge case tests

Great work on this feature! The versioning support is a valuable addition to the framework. 🚀

Code Review: API Versioning Implementation

Summary

This PR implements a comprehensive API versioning feature for @mcp-apps-kit/core, allowing multiple API versions to be exposed from a single application. The implementation is well-architected with good test coverage and maintains backward compatibility.

✅ Strengths

1. Architecture & Design

• Clean separation: Multi-version and single-version apps are handled through separate code paths while sharing core infrastructure
• Shared Express app: Efficient use of a single Express instance for all versions reduces memory overhead
• Lazy OAuth initialization: Smart use of getter functions for JWKS clients enables proper initialization in both server and serverless contexts
• Deep merge strategy: Well-thought-out config merging with null/undefined semantics (null = disable, undefined = inherit)

2. Type Safety

• Strong TypeScript usage throughout with proper generic constraints
• Version-specific type exports in examples (AppClientToolsV1, AppClientToolsV2)
• Good use of type guards (isVersionsConfig, isUIDef)

3. Testing

• Comprehensive test suite covering:
  • Multi-version creation and isolation
  • Config merging (deep merge, null handling, array replacement)
  • Route isolation and tool execution
  • Backward compatibility
  • Serverless (handleRequest) scenarios
• 967 lines of well-structured tests demonstrate thorough validation

4. Documentation

• Excellent documentation updates in README and package README
• Clear examples showing v1 vs v2 differences
• Well-documented minimal example with versioning

5. Error Handling

• Proper validation of version keys (/^v\d+$/)
• Conflict detection for reserved routes (/health)
• Clear error messages with available versions when accessing non-existent versions

🔍 Issues & Concerns

Critical Issues

1. Version Key Constraint Too Restrictive

Location: packages/core/src/createApp.ts:84-91

function validateVersionKey(versionKey: string): void {
  if (!/^v\d+$/.test(versionKey)) {
    throw new AppError(
      ErrorCode.INVALID_CONFIG,
      `Version key must match pattern /^v\\d+$/, got: "${versionKey}"`
    );
  }
}

Issue: The pattern /^v\d+$/ only allows simple numeric versions (v1, v2, v10) but prevents common versioning patterns:

• ❌ v1.0, v2.1 (minor versions)
• ❌ v1-beta, v2-alpha (pre-release versions)
• ❌ v2023, v2024 (date-based versions)

Impact: Limits flexibility for real-world API versioning strategies.

Recommendation: Consider relaxing to /^v\d+(\.\d+)?(-\w+)?$/ or document the rationale for the restrictive pattern. If intentional, add a comment explaining why only major versions are supported.

2. OAuth JWKS Client Shared Across Versions

Location: packages/core/src/createApp.ts:803-855

// Create version-specific OAuth JWKS client key (for reuse if config is identical)
const oauthConfigKey = normalizedVersionConfig.config?.oauth
  ? JSON.stringify(normalizedVersionConfig.config.oauth)
  : "no-oauth";

Issue: Using JSON.stringify() for cache key generation has risks:

• Property order sensitivity: {a:1, b:2} !== {b:2, a:1} in JSON.stringify
• Function serialization: Loses tokenVerifier function if present
• Nested object issues: May not handle complex nested structures consistently

Example failure case:

// These should be identical but create different keys:
{ authorizationServer: "https://auth.com", scopes: ["read"] }
{ scopes: ["read"], authorizationServer: "https://auth.com" }

Recommendation: Use a stable hash function or compare normalized config objects:

const oauthConfigKey = normalizedVersionConfig.config?.oauth
  ? crypto
      .createHash('sha256')
      .update(
        JSON.stringify(
          Object.keys(config.oauth)
            .sort()
            .reduce((acc, key) => ({ ...acc, [key]: config.oauth[key] }), {})
        )
      )
      .digest('hex')
  : "no-oauth";

3. Missing Validation for serverRoute in Versioned Config

Location: packages/core/src/createApp.ts:139-156

if (serverRoute === "/health") {
  throw new AppError(
    ErrorCode.INVALID_CONFIG,
    `${prefix}.serverRoute cannot be "/health" as it conflicts with the health check endpoint`
  );
}

Issue: The validation prevents /health conflict, but multi-version apps use /{versionKey}/mcp routes that are hardcoded. If a version tries to override serverRoute, it will be ignored but no warning is issued.

Recommendation: Add validation or warning when serverRoute is specified in version-specific config:

if (versionConfig.config?.serverRoute !== undefined) {
  throw new AppError(
    ErrorCode.INVALID_CONFIG,
    `Version "${versionKey}".config.serverRoute is not supported in multi-version apps. Routes are automatically assigned as /{versionKey}/mcp`
  );
}

Medium Issues

4. Debug Logger Not Configured Per-Version

Location: packages/core/src/createApp.ts:788-791

// Note: Debug logger is configured once with global config (line 681-683).
// We don't reconfigure it per-version because it's a global singleton.

Issue: This is documented but may lead to unexpected behavior. If v1 has debug.level: "error" and v2 has debug.level: "debug", the global logger will use whichever was configured first.

Recommendation: Either:

1. Document this limitation prominently in the versioning docs
2. Implement per-version logger instances (breaking change)
3. Merge debug configs and use the most verbose level across all versions

5. Type Inference Issue for Multi-Version Apps

Location: packages/core/src/createApp.ts:1010-1012

tools: (Object.values(config.versions)[0] as VersionConfig<T> | undefined)?.tools as T,

Issue: The main app's tools property uses the first version's tools for type inference. This is confusing because:

• It's arbitrary (depends on object key order)
• Users might expect to access all versions' tools
• The cast to T loses type safety

Recommendation: Document this clearly or provide a discriminated union type for multi-version tool access:

// Better API:
const v1App = app.getVersion("v1");
const result = await v1App.tools.greet({ name: "Alice" }); // Type-safe

6. Event Deduplication in once()

Location: packages/core/src/createApp.ts:1146-1189

Issue: The once() implementation for multi-version apps is complex and has race condition considerations. While the current implementation looks correct, it's prone to subtle bugs.

Observation: Good defensive programming with the isUnsubscribed flag to prevent race conditions. However, the complexity suggests this might be a code smell.

Recommendation: Add integration tests specifically for concurrent event firing across versions to validate the race condition handling.

Minor Issues

7. Inconsistent Error Messages

// packages/core/src/createApp.ts:102
`Version "${versionKey}".version is required and must be a non-empty string`

// packages/core/src/createApp.ts:109
`Version "${versionKey}".tools is required and must be an object`

Issue: Inconsistent use of .version vs .tools in error messages could be confused with the version string itself.

Recommendation: Use bracket notation for clarity:

`Version "${versionKey}"["version"] is required...`

8. Missing JSDoc for Public Methods

Location: packages/core/src/createApp.ts:726-736

getVersion: (_versionKey: string): App<T> | undefined => {
  return undefined;
},

Issue: Public API methods getVersion() and getVersions() lack JSDoc documentation in the implementation.

Recommendation: Add JSDoc comments to match the style used elsewhere in the codebase.

9. Hardcoded Port in Tests

Location: packages/core/tests/unit/versioning.test.ts (multiple locations)

const port = 3100; // 3101, 3102, 3103, 3104, 3105...

Issue: Sequential hardcoded ports could cause test flakiness if:

• Tests run in parallel
• Ports are already in use on CI
• Tests don't clean up properly

Recommendation: Use port 0 to let the OS assign random available ports:

const port = 0; // Let OS assign
await app.start({ port });
const actualPort = app.getServer().httpServer?.address().port;

10. Missing Test for Config Override Precedence

The tests cover deep merge scenarios but don't test the precedence explicitly when both global and version configs exist for complex scenarios.

Recommendation: Add a test showing explicit precedence:

it("should give version config precedence over global config", () => {
  // Global says protocol: "mcp", cors.origin: true
  // v1 says protocol: "openai", cors.credentials: true
  // Verify v1 has: protocol="openai", cors.origin=true, cors.credentials=true
});

🔒 Security Considerations

✅ Well Handled

1. OAuth isolation: Each version can have independent OAuth configuration
2. JWKS client reuse: Proper caching prevents redundant network requests
3. Bearer token validation: Consistent validation across versions
4. Route isolation: Versions cannot interfere with each other's tools

⚠️ Considerations

1. JWKS cache sharing: Versions with different authorizationServer URLs share JWKS client cache. While efficient, ensure this doesn't leak information if versions should be isolated.
2. Error messages: Version-not-found errors expose all available versions (availableVersions). This is fine for most use cases but could be information disclosure in high-security environments.

🚀 Performance Considerations

✅ Well Optimized

1. Single Express app: Avoids HTTP server overhead per version
2. Shared middleware: Global middleware registered once, not per-version
3. Lazy OAuth init: Defers JWKS discovery until first request in serverless
4. JWKS client pooling: Reuses clients for identical OAuth configs

💡 Potential Optimizations

1. Plugin initialization: Currently sequential (await in loop at line 1020). Consider Promise.all() for parallel initialization
2. Event subscriptions: Multi-version apps subscribe to all versions' events. For high-volume apps, this could create many listeners

📋 Recommendations Summary

Must Fix Before Merge

1. ⚠️ Fix OAuth config cache key generation (use stable hash)
2. ⚠️ Add validation/warning for version-specific serverRoute overrides

Should Fix

1. Document debug logger global singleton limitation
2. Document or improve main app tools type inference for multi-version
3. Consider relaxing version key pattern or document rationale

Nice to Have

1. Use dynamic ports in tests (port 0)
2. Add integration tests for event race conditions
3. Add explicit config precedence test
4. Add JSDoc to public methods
5. Consider parallel plugin initialization

✨ Overall Assessment

Grade: A- (Excellent with minor issues)

This is a well-implemented feature with:

• ✅ Strong architecture and design patterns
• ✅ Comprehensive test coverage (967 lines)
• ✅ Excellent documentation
• ✅ Backward compatibility maintained
• ✅ Good TypeScript usage
• ⚠️ A few fixable issues around config hashing and validation

The implementation shows maturity and production-readiness. The critical issues are fixable with small changes and don't represent fundamental design flaws.

Recommendation: Request changes for the two critical issues, then approve after fixes.

Great work on this feature! The versioning system is well-thought-out and will be valuable for evolving APIs. 🎉

Code Review: API Versioning Implementation

Thank you for this substantial feature addition! This PR implements API versioning support for MCP AppsKit, allowing multiple API versions to be exposed from a single application. Here's my detailed review:

✅ Strengths

1. Excellent Architecture & Design

• Clean separation of concerns: Single-version and multi-version apps are handled elegantly with shared code paths
• Backward compatibility: Existing single-version apps continue to work without changes - excellent design decision
• Deep merge strategy: The deepMerge function with null-to-disable semantics is well-thought-out and powerful
• Type safety: Strong TypeScript usage with proper type inference and DeepPartialWithNull<T> utility type

2. Comprehensive Documentation

• README updates clearly explain versioning concepts with practical examples
• Package-level documentation is thorough
• Inline comments explain complex logic (e.g., OAuth initialization, route mounting)
• Example app demonstrates real-world usage effectively

3. Robust Testing

• Unit tests cover config validation, merging, route isolation, and backward compatibility
• Tests verify version key validation and serverRoute conflicts
• Good coverage of edge cases

4. Security & Best Practices

• OAuth JWKS client reuse across versions with identical configs (line 820-825 in createApp.ts)
• Version key validation prevents injection attacks (/^v\d+$/)
• Proper error handling throughout
• Idempotent OAuth initialization

🔍 Issues & Concerns

Critical Issues

1. Resource Leak in Multi-Version Apps ⚠️

// packages/core/src/createApp.ts:899
// Express strips the mount path, so routes registered at serverRoute on the version app

The comment ends mid-sentence, suggesting incomplete implementation. More importantly, I don't see cleanup logic for:

• Event emitters when versions are modified
• Plugin managers per version
• Middleware chains per version

Recommendation: Implement a dispose() or stop() method for multi-version apps that cleans up all version-specific resources.

2. Debug Logger Singleton Issue 🐛

// packages/core/src/createApp.ts:788-791
// Note: Debug logger is configured once with global config (line 681-683).
// We don't reconfigure it per-version because it's a global singleton.

This is explicitly acknowledged but creates a limitation:

• Version-specific debug configs are ignored
• All versions share the same debug logger configuration
• If v1 needs level: "error" and v2 needs level: "debug", this doesn't work

Recommendation: Either:

1. Document this limitation clearly in the versioning docs
2. Make the debug logger context-aware (accepts a version identifier)
3. Consider per-version logger instances

3. Missing Health Check Version Information 📋

// packages/core/src/server/index.ts:250-252
expressApp.get("/health", (_req: Request, res: Response) => {
  res.json({ status: "ok", name: config.name, version: config.version });
});

For single-version apps, this works fine. But for multi-version apps, the shared health endpoint should return ALL available versions, not just one. According to the PR description, it should "return all available versions".

Expected behavior:

{
  "status": "ok",
  "name": "my-app",
  "versions": [
    { "key": "v1", "version": "1.0.0", "route": "/v1/mcp" },
    { "key": "v2", "version": "2.0.0", "route": "/v2/mcp" }
  ]
}

4. Incomplete Route Implementation 🔧
Line 899 in createApp.ts has a truncated comment that suggests incomplete implementation:

// Note: Express strips the mount path, so routes registered at serverRoute on the version app

This needs to be completed or removed.

Medium Priority Issues

5. Inconsistent OAuth Audience Handling

// packages/core/src/server/index.ts:139-145
const protectedResourceUrl = new URL(serverRoute, config.config.oauth.protectedResource);
const oauthConfigWithAudience = {
  ...config.config.oauth,
  audience: config.config.oauth.audience ?? protectedResourceUrl.href,
};

For multi-version apps with different OAuth configs per version, the audience should include the version route (/v1/mcp, /v2/mcp). Currently, all versions might share the same base audience, which could cause validation issues.

Recommendation: Ensure version-specific OAuth audiences are properly constructed when OAuth configs differ per version.

6. Potential Memory Issue with JWKS Client Caching

// packages/core/src/createApp.ts:804-806
const oauthConfigKey = normalizedVersionConfig.config?.oauth
  ? JSON.stringify(normalizedVersionConfig.config.oauth)
  : "no-oauth";

Using JSON.stringify for cache keys can be fragile:

• Property ordering might differ, causing cache misses
• Large OAuth configs create large cache keys
• No cleanup mechanism for cached JWKS clients

Recommendation: Consider using a hash function (crypto.createHash) or implement proper cache key normalization.

7. Missing Validation for Conflicting Version Routes
Version keys must match /^v\d+$/, but there's no validation to prevent:

• A version with config.serverRoute = "/v2/mcp" when v2 exists
• Custom serverRoutes that conflict with other version routes
• Routes that conflict with reserved endpoints (/health, /.well-known/*)

Recommendation: Add validation in validateVersionConfig to detect route conflicts.

Low Priority / Code Quality

8. Type Assertions Could Be Improved

// packages/core/src/createApp.ts:414-434
const mergedConfig: GlobalConfig = {
  oauth: deepMerge(...) as GlobalConfig["oauth"],
  cors: deepMerge(...) as GlobalConfig["cors"],
  // ...
};

Multiple as assertions suggest the type inference could be improved. Consider making deepMerge generic with better type constraints.

9. Test Coverage Gaps
Looking at versioning.test.ts, I notice:

• No integration tests for actual HTTP requests to versioned endpoints
• No tests for OAuth with different configs per version
• No tests for plugin hooks in multi-version context
• No tests for concurrent requests to different versions

Recommendation: Add integration tests that start a server and make actual HTTP calls.

10. Example Code Quality

// examples/minimal/src/index.ts:53
},
});

// =============================================================================
// V2: Enhanced greet tool (name + optional surname)
// =============================================================================

The example is well-structured, but could be enhanced:

• Add error handling in the UI components
• Show how to handle version-specific errors
• Demonstrate middleware per version

📝 Minor Issues

1. Typo in comment (line 788): "line 681-683" should be updated to reflect actual line numbers after refactoring
2. Inconsistent formatting: Some sections use // === separators, others don't
3. Missing JSDoc: Several exported functions lack JSDoc comments (e.g., mergeVersionConfig)
4. Console.log in production code: The example uses console.log which should use the debug logger

🎯 Recommendations

Must Fix Before Merge:

1. ✅ Complete the truncated comment at line 899
2. ✅ Fix health endpoint to return all versions for multi-version apps
3. ✅ Document the debug logger singleton limitation
4. ✅ Add resource cleanup mechanism for multi-version apps

Should Fix:

5. Add route conflict validation
6. Improve OAuth audience handling for versioned apps
7. Use hash-based cache keys instead of JSON.stringify
8. Add integration tests

Nice to Have:

9. Improve type inference to reduce as assertions
10. Add more example scenarios (error handling, versioned middleware)
11. Consider adding a migration guide for existing apps

🚀 Overall Assessment

This is a well-designed and implemented feature that adds significant value to the framework. The core architecture is sound, backward compatibility is maintained, and the documentation is comprehensive.

Score: 8/10

The main issues are:

• Incomplete implementation details (truncated comment, health endpoint)
• Debug logger limitation that affects usability
• Missing resource cleanup
• Some edge cases not fully handled

Once the critical issues are addressed, this will be an excellent addition to the framework.

Recommendation: Request Changes - The issues are fixable and not fundamental flaws.

💡 Additional Suggestions

1. Consider adding a versioning migration tool: Help users upgrade from v1 to v2 by showing diffs
2. Version deprecation strategy: Add metadata to mark versions as deprecated
3. Version routing middleware: Allow custom version resolution (e.g., from headers)
4. OpenAPI/Swagger generation: Auto-generate API docs per version

Great work overall! Looking forward to seeing this feature land. 🎉

Code Review: API Versioning Implementation

Summary

This PR implements API versioning support for mcp-apps-kit, allowing a single application to expose multiple API versions at different routes (e.g., /v1/mcp, /v2/mcp). The implementation is comprehensive, well-tested, and maintains backward compatibility with single-version apps.

✅ Strengths

1. Excellent Architecture & Design

• Clean separation between single-version and multi-version app creation
• Smart config merging with deep merge logic for nested objects
• Proper use of null vs undefined semantics (null = disable, undefined = inherit)
• Shared Express app across versions reduces resource overhead
• Lazy JWKS client initialization for OAuth supports multi-version scenarios efficiently

2. Strong Type Safety

• Comprehensive TypeScript types with proper inference
• VersionsConfig, VersionConfig, and DeepPartialWithNull types are well-designed
• Validation functions with clear error messages
• Type-safe version key pattern enforcement (/^v\d+$/)

3. Robust Testing

• Extensive test coverage (967 lines of tests!)
• Tests cover: validation, config merging, route isolation, middleware, backward compatibility, serverless
• Integration tests verify actual HTTP endpoints work correctly
• Good edge case coverage (404s, invalid versions, etc.)

4. Backward Compatibility

• Single-version apps continue to work without changes
• getVersions() returns empty array for single-version apps
• getVersion() returns undefined for single-version apps
• Clear migration path for existing users

5. Documentation

• Comprehensive README updates with examples
• Updated minimal example demonstrates versioning in practice
• Clear API documentation for new methods

⚠️ Issues & Concerns

1. Critical: OAuth Middleware Pattern (Security)

Location: packages/core/src/createApp.ts:615, 888

The OAuth middleware receives a lazy getter for the JWKS client. The middleware needs to handle the case where getJwksClient() returns null during a request. If OAuth initialization fails or is delayed, requests could be processed without proper authentication.

Recommendation: Add explicit null checks in the OAuth middleware before token verification, consider failing fast with a 503 Service Unavailable if OAuth is configured but JWKS client isn't initialized.

2. Potential Bug: Event Handling Race Condition

Location: packages/core/src/createApp.ts:1150-1192

The once() implementation for multi-version apps has potential race condition where multiple versions could emit the same event simultaneously, causing the handler to execute multiple times despite the once semantics.

Recommendation: Use atomic compare-and-swap pattern or add integration test that fires events from multiple versions concurrently.

3. Config Validation: Missing Edge Cases

Location: packages/core/src/createApp.ts:84-91, 153-156

Version key validation rejects /health as serverRoute but doesn't validate conflicts with /.well-known/openai-apps-challenge or potential version key mount point conflicts.

Recommendation: Add validation for reserved routes and conflicting version paths.

4. Performance: Deep Merge Complexity

Location: packages/core/src/createApp.ts:341-388

The deepMerge function lacks protection against circular references and has no depth limit, potentially causing stack overflow with deeply nested configs.

Recommendation: Add max depth check (e.g., depth > 10 throws error) or consider using a well-tested library like lodash.merge.

5. Code Smell: Type Assertions

Location: packages/core/src/createApp.ts:419-438

Multiple type assertions in config merging reduce type safety. Consider using generics to preserve types through deepMerge or use Zod for runtime validation with type inference.

6. Potential Memory Leak: Server Cleanup

Location: packages/core/src/createApp.ts:759-1215

Multi-version apps create multiple server instances, plugin managers, middleware chains, and event emitters. There's no explicit cleanup/shutdown method for the multi-version main app.

Recommendation: Add a shutdown() method to properly cleanup resources.

💡 Suggestions for Enhancement

1. Version Negotiation: Consider adding content negotiation support via Accept headers
2. Version Deprecation: Support deprecation warnings and sunset dates per version
3. Health Check Improvements: Include per-version status and uptime information
4. Version-Specific Rate Limiting: Allow different rate limits per version

🔍 Minor Issues

1. Debug Logger: Multi-version apps configure debug logger once globally (line 772-774) but version-specific debug configs are ignored. Document this limitation.
2. Missing Export: VersionsConfig and VersionConfig types should be explicitly exported in packages/core/src/index.ts.
3. Example Code: Consider adding vite config examples for multi-UI builds in the minimal example.

📊 Test Coverage Suggestions

• Test OAuth with different configs per version
• Test plugin execution order (global + version-specific)
• Test concurrent version initialization
• Test middleware execution isolation between versions

🎯 Overall Assessment

Quality: ⭐⭐⭐⭐½ (4.5/5)

This is a well-designed, thoroughly tested feature implementation. The architecture is sound, backward compatibility is maintained, and the documentation is comprehensive.

Main Concerns:

1. OAuth lazy initialization safety (needs verification)
2. Event handler race condition in once()
3. Deep merge edge cases and performance
4. Missing shutdown/cleanup for multi-version apps

Recommendation: ✅ Approve with minor changes

The issues identified are mostly edge cases and can be addressed incrementally. The core implementation is solid and ready for merge after:

1. Adding null checks in OAuth middleware for lazy client getter
2. Adding circular reference protection to deepMerge
3. Documenting the debug logger limitation for multi-version apps
4. (Optional but recommended) Adding a shutdown() method

Great work! This is a valuable addition to the framework. 🚀

Code Review: API Versioning Implementation

This PR introduces a comprehensive API versioning feature to mcp-apps-kit, enabling multiple API versions from a single application. Overall, this is a well-designed and thoroughly implemented feature with excellent test coverage.

✅ Strengths

1. Excellent Architecture & Design

• Clean abstraction: The versioning implementation cleanly separates single-version and multi-version apps while maintaining backward compatibility
• Type safety: Strong TypeScript typing throughout with proper generic constraints
• Deep merge semantics: The null/undefined/value semantics for config merging is well-designed and intuitive
• Shared infrastructure: Smart reuse of Express app and OAuth JWKS clients across versions reduces memory footprint

2. Outstanding Test Coverage

• 967 lines of tests covering edge cases, config merging, route isolation, middleware, and backward compatibility
• Tests use real MCP client connections, validating end-to-end behavior
• Integration tests verify actual HTTP behavior, not just unit logic

3. Documentation Quality

• Comprehensive updates to README and package docs
• Clear examples in both root README and minimal example
• Good inline code comments explaining complex logic

4. Backward Compatibility

• Single-version apps continue to work unchanged
• getVersions() and getVersion() gracefully return empty/undefined for single-version apps

🔧 Issues & Recommendations

1. Critical: OAuth Lazy Initialization Race Condition ⚠️

Location: packages/core/src/createApp.ts:825-889

Problem: If the initialization promise fails and is deleted from the map (line 882), a concurrent call could get undefined from jwksClients.get() and return without setting versionJwksClient, leaving it null.

Recommendation: Add error handling to ensure versionJwksClient is set or throw an error.

2. Type Safety: Unsafe Type Assertions in mergeVersionConfig

Location: packages/core/src/createApp.ts:429-444

Issue: Multiple as type assertions on deepMerge results bypass type checking. Runtime type errors if deepMerge returns unexpected types won't be caught.

Recommendation: Add runtime validation or make deepMerge generic with proper constraints.

3. Documentation: Missing Migration Guide

Issue: No explicit migration guide for users updating from older versions.

Recommendation: Add a Migration section to the README covering:

• How to migrate single-version apps to multi-version
• When to use versioning vs. not
• Performance considerations for multi-version apps

4. Testing: Missing OAuth Error Scenarios

Issue: Tests don't cover OAuth initialization failures in multi-version apps.

Recommendation: Add test cases for:

• Invalid authorization server URL
• JWKS discovery timeout/failure
• Concurrent version initialization with OAuth

5. Type Safety: mainApp.tools Type Inference

Location: packages/core/src/createApp.ts:1022

Issue: This uses tools from the first version for type inference, which may not match other versions tools.

Recommendation: Document this limitation clearly or consider a union type approach.

📊 Summary

✅ Approval Recommendation

LGTM with minor fixes recommended

This is a high-quality implementation that adds significant value to the framework. The critical OAuth race condition should be addressed before merge, but overall this is excellent work.

Pre-merge Checklist:

• Fix OAuth lazy initialization race condition (Critical)
• Improve type safety in mergeVersionConfig (Recommended)
• Add OAuth failure test cases (Recommended)
• Clarify serverRoute behavior in multi-version mode (Documentation)

Great job on this feature! 🎉